Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

addrs: Central implementation of sets and maps with UniqueKeyer address types #31238

Merged
merged 3 commits into from
Jun 16, 2022

Conversation

apparentlymart
Copy link
Contributor

@apparentlymart apparentlymart commented Jun 13, 2022

The addrs.UniqueKey and addrs.UniqueKeyer mechanism was a compromise we introduced as part of the config-driven move project as a way to make maps with addresses as keys even though a lot of our address types aren't comparable.

We had previously done this typically by using string representations of the address types (.String()) directly as map keys, which works okay most of the time but runs into trouble in situations where the string representations are ambiguous, such as that an addrs.ResourceInstance with no key and an addrs.Resource can have an identical string representation. The addrs.UniqueKey idea allowed us to dynamically retain distinctions between the address types internally even if we are still ultimately doing just string comparisons for some of them, because all of the address types have distinct implementations of addrs.UniqueKey and thus are treated as distinct keys in a Go map.

However, that compromise still didn't address the fact that the map key types could not be statically checked by the compiler, making it easy to erroneously put a key of the wrong type into a map and have that cause erratic behavior at runtime rather than a compile-time error.

This PR therefore completes what I previously attempted with addrs.Set (which I had actually not intended to include in the addrs.UniqueKey PR before; whoops!), now making use of Go 1.18 type parameter syntax to define generic addrs.Set and addrs.Map types that can take specific addrs.UniqueKeyer implementations as their key types, with write operations checked at compile time.

The main event here is actually addrs.Map, which can store a value of a chosen type alongside each element. Unfortunately in the process we lose the ability to use standard Go indexing syntax to work with the map, and so we need to use a method-based API instead. I converted the refactoring package to use addrs.Map to see if it still felt reasonably ergonomic, and I subjectively think it's still readable enough and therefore a net win when considering the benefit of type safety, but of course that's my own bias towards compile-time checks that others might not share. (The original motivation for the addrs package was to allow compile-time checking of address types, where beforehand we were just passing specially-formatted string values everywhere and routinely getting their "types" confused, so this at least seems consistent with that mission.)

I decided to leave the internals of these structures exported rather than hidden as a matter of pragmatism, because it brings the following benefits:

  • We can use our typical approaches like cmp.Equal and cmp.Diff with these data structures in tests without the need for any special extra cmp options.

  • For addrs.Set, it's reasonable to range directly over the values of the set to efficiently visit them all without any copying. It could perhaps be reasonable to do this with the Elems field of addrs.Map too, but that would expose the caller to the MapElement type, so I guess we'll see from experience whether that's actually useful and a good idea.

    (Note: The addrs.Map is intentionally a struct with a map field inside it rather than just a named type directly for a map type because with the latter I found the compiler gave confusing feedback if I accidentally used the normal Go indexing syntax with an addrs.Map value, because it looked like a type error on the key rather than reporting that it's a type that doesn't support indexing at all.)

As noted in the documentation comments, callers should not directly modify the internals even though the compiler would allow it. The internals are exposed for reading only.


I worked on this now really because I'm about to start working on something else that will, I hope, use of both addrs.Set and addrs.Map as part of its implementation. I wanted to get feedback on this in isolation first though, using the refactoring package changes to explore the ergonomics of it, so that if we decide to change the design here or to not move forward with it at all then I can avoid the need to retroactively replace uses of these new types in my ongoing project work.

The changes to the refactoring package here should be just a mechanical replacement of one usage pattern with another, with no change in behavior. If anything looks like it's doing something materially different than the code it replaced then that's a mistake on my part, so please point it out!

@apparentlymart
Copy link
Contributor Author

It looks like our "Code Consistency Checks" step is tripping over something here, possibly because we need newer versions of the generators like stringer in order to get type parameters support. I'll look into that shortly, but it doesn't seem like it's flagging anything wrong with the code I changed here in particular so I think it's still reviewable and discussable in the meantime.

@apparentlymart apparentlymart force-pushed the f-addrs-generic-uniquekey branch 2 times, most recently from 92f45c6 to 287e4e0 Compare June 13, 2022 20:46
@apparentlymart
Copy link
Contributor Author

apparentlymart commented Jun 13, 2022

Okay... looks like upgrading the golang.org/x/tools module to get a newer stringer was sufficient to make go generate work again. 😌

This means we can now use type parameter syntax where appropriate.

This commit also includes an upgrade to the golang.org/x/tools module,
in order to get a newer version of "stringer" that supports the type
parameters syntax.
The addrs.Set type previously snuck in accidentally as part of the work
to add addrs.UniqueKey and addrs.UniqueKeyer, because without support for
generic types the addrs.Set type was a bit of a safety hazard due to not
being able to enforce particular address types at compile time.

However, with Go 1.18 adding support for type parameters we can now turn
addrs.Set into a generic type over any specific addrs.UniqueKeyer type,
and complement it with an addrs.Map type which supports addrs.UniqueKeyer
keys as a way to encapsulate the handling of maps with UniqueKey keys that
we currently do inline in various other parts of Terraform.

This doesn't yet introduce any callers of these types, but we'll convert
existing users of addrs.UniqueKeyer gradually in subsequent commits.
We introduced the addrs.UniqueKey and addrs.UniqueKeyer mechanics as part
of implementing the ValidateMoves and ApplyMoves functions, as a way to
better encapsulate the solution to the problem that lots of our address
types aren't comparable and so cannot be used directly as map keys.

However, exposing addrs.UniqueKey handling directly in the logic adds
various noise to the algorithms and, in particular, obscures the fact that
MoveResults.Changes and MoveResult.Blocked both have different map key
types.

Here then we'll use the new addrs.Map helper type, which encapsulates the
idea of a map from an addrs.UniqueKeyer type to an arbitrary value type,
using the unique keys as the map keys internally. This does unfortunately
mean that we lose the conventional Go map access syntax and have to use
a method-based API instead, but I (subjectively) think that's an okay
compromise in return for avoiding the need to keep track inline of which
addrs.UniqueKey values correspond with which real addresses.

This is intended as an entirely-mechanical change, with equivalent
behavior to what it replaced. If anything here is doing something
materially different than what it replaced then that's a mistake.
Copy link
Member

@jbardin jbardin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think this is a great first use of type parameters, since we have so many maps/sets of these data types.

@apparentlymart apparentlymart merged commit dc5964f into main Jun 16, 2022
@apparentlymart apparentlymart deleted the f-addrs-generic-uniquekey branch June 16, 2022 14:03
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants